Skip to content

add diagnostics subcommand to rust-analyzer CLI #3971

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Apr 14, 2020
Merged

add diagnostics subcommand to rust-analyzer CLI #3971

merged 3 commits into from
Apr 14, 2020

Conversation

@JoshMcguigan
Copy link
Contributor

@JoshMcguigan JoshMcguigan commented Apr 14, 2020

This PR adds a diagnostics subcommand to the rust-analyzer CLI. The intent is to detect all diagnostics on a workspace. It returns a non-zero status code if any error diagnostics are detected. Ideally I'd like to run this in CI against the rust analyzer project as a guard against false positives.

$ cargo run --release --bin rust-analyzer -- diagnostics .

Questions for reviewers:

  1. Is this the proper way to get all diagnostics for a workspace? It seems there are at least a few ways this can be done, and I'm not sure if this is the most appropriate mechanism to do this.
  2. It currently prints out the relative file path as it is collecting diagnostics, but it doesn't print the crate name. Since the file name is relative to the crate there can be repeated names, so it would be nice to print some identifier for the crate as well, but it wasn't clear to me how best to accomplish this.

@flodiebold
Copy link
Member

flodiebold commented Apr 14, 2020

it would be nice to print some identifier for the crate as well, but it wasn't clear to me how best to accomplish this.

We have a display_name in CrateData nowadays for this purpose. Though it may be nicer to have a file path from the source root for generating full file names 🤔 In any case, we can fix this in a separate PR, analysis-stats has the same problem.

flodiebold
flodiebold reviewed Apr 14, 2020
View reviewed changes
@flodiebold
Copy link
Member

flodiebold commented Apr 14, 2020 •
edited
Loading

Ideally I'd like to run this in CI against the rust analyzer project as a guard against false positives.

I'd be a bit wary about that at this stage, we don't want random PRs to break because they added new code that happens to have a false positive 😬 (Apart from the impact on build times...)

Since I don't have a much better approach for getting all diagnostics, I'd be fine with merging this one for now, considering this is just a debug tool anyway at the moment.

bors d+

@bors
Copy link
Contributor

bors bot commented Apr 14, 2020

✌️ JoshMcguigan can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@JoshMcguigan
Copy link
Contributor Author

JoshMcguigan commented Apr 14, 2020

@flodiebold I've changed this quite a bit in my most recent commit based on your suggestions. If you don't mind I'd prefer you take another quick look before I merge.

@JoshMcguigan
Copy link
Contributor Author

JoshMcguigan commented Apr 14, 2020

I'd be a bit wary about that at this stage, we don't want random PRs to break because they added new code that happens to have a false positive grimacing (Apart from the impact on build times...)

This is a good point. My thought was this would run nightly, similar to analysis-stats.

flodiebold
flodiebold reviewed Apr 14, 2020
View reviewed changes
@JoshMcguigan
Copy link
Contributor Author

JoshMcguigan commented Apr 14, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 14, 2020

Build succeeded:

@bors bors bot merged commit b495e56 into rust-lang:master Apr 14, 2020
matklad
matklad reviewed Apr 15, 2020
View reviewed changes
crates/rust-analyzer/src/cli/diagnostics.rs

if found_error {
println!();
Err(anyhow!("diagnostic error detected"))
Copy link
Contributor

@matklad matklad Apr 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor point, but I prefer spelling this as format_err instead (the same macro is exported under two aliases).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@flodiebold flodiebold flodiebold left review comments

+1 more reviewer

@matklad matklad matklad left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JoshMcguigan @flodiebold @matklad